Implementation of matplotlib backend for criterion_plot()
#599
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
r3kste
changed the title
matplotlib backend for criterion_plot()matplotlib backend for criterion_plot()
r3kste
force-pushed
the
backend_plotting
branch
4 times, most recently
from
556334a to
d0c6856
Compare
There was a problem hiding this comment.
Thanks a lot! I have a few comments, if you have any questions let me know here or via Zulip. I think it will be important to get this PR right such that we don't complicate our lives when handling the other visualization functions.
My main concern is that I'd like the usage of the plotting class to feel more natural. See one of my comments in the backends module.
On an abstract level, a plotting class supporting the criterion plot needs to be able to:
- Create a figure
- Add lines to that figure
- Set a template
- Set a legend position
- Set axis labels
Now I would expect that our plotting class somehow incorporates / supports these actions or an action that combines a subset of them (e.g., if we think that some of these actions comprise one block, like we currently do with the PlotConfig).
Additionally
- As I am saying in one of the comments, we need to check whether we actually need the PlotConfig class.
- I thought about your proposal to rename
OptimizeResultOrPathtoResultOrPath, and I think I prefer that! Could you change that?
Thanks a lot!!
| class BackendRegistry: | ||
| _registry: dict[str, type[BackendWrapper]] = {} | ||
|
|
||
| @classmethod | ||
| def register(cls, backend_name: str) -> Callable: | ||
| def decorator(backend_wrapper): | ||
| cls._registry[backend_name] = backend_wrapper | ||
| return backend_wrapper | ||
|
|
||
| return decorator | ||
|
|
||
| @classmethod | ||
| def get_backend_wrapper(cls, backend_name: str) -> type[BackendWrapper]: | ||
| if backend_name not in cls._registry: | ||
| raise ValueError( | ||
| f"Backend '{backend_name}' is not supported. " | ||
| f"Supported backends are: {', '.join(cls._registry.keys())}." | ||
| ) | ||
| return cls._registry[backend_name] |
There was a problem hiding this comment.
While this BackendRegistry is pretty cool I think we can replace it by a registry dictionary and a function to reduce complexity. Let me know what you think about the trade-offs here.
def get_plotting_backend_class(backend: str) -> PlottingWrapper:
backend_to_class = {
"plotly": PlotlyBackend,
"matplotlib": MatplotlibBackend,
}
if backend not in backend_to_class:
msg = (
f"Backend '{backend}' is not supported. Supported backends are: "
f"{', '.join(backend_to_class.keys())}."
)
raise ValueError(msg)
return backend_to_class[backend]There was a problem hiding this comment.
def get_plotting_backend_class(backend: str) -> PlottingWrapper: backend_to_class = { "plotly": PlotlyBackend, "matplotlib": MatplotlibBackend, }
Is there any specific reason why the dictionary should be defined within the function rather than as a global object?
| get_palette_cycle, | ||
| ) | ||
|
|
||
| _criterion_plot_legend: dict[str, dict[str, Any]] = { |
There was a problem hiding this comment.
| _criterion_plot_legend: dict[str, dict[str, Any]] = { | |
| CRITERION_PLOT_BACKEND_TO_LEGEND: dict[str, dict[str, Any]] = { |
| class BackendWrapper(abc.ABC): | ||
| default_template: str | ||
| default_palette: list | ||
|
|
||
| def __init__(self, plot_config: PlotConfig): | ||
| self.plot_config = plot_config | ||
|
|
||
| @abc.abstractmethod | ||
| def line_plot(self, lines: list[LineData]) -> None: | ||
| pass | ||
|
|
||
| @abc.abstractmethod | ||
| def label(self, **kwargs): | ||
| pass | ||
|
|
||
| @abc.abstractmethod | ||
| def return_obj(self): | ||
| pass |
There was a problem hiding this comment.
I am not entirely convinced by the methods and their names of this class.
line_plotis not returning a plot, but adding lines to a figure that is created in the init. It should probably be calledadd_lines?- The
labelmethod also updated the config in the plotly class and the legend in the matplotlib class. I think it should be calledset_labelsand only update the labels. - To update the legend we should probably add a
set_legend_positionmethod. - Where do we need to set the template? Can we set the template in the init?
- Why do we need a PlotConfig object now? Could we do without it?
I think in particular I would like the usage of the class to be easier and more natural. Something in the direction of:
plot = plot_cls(template)
plot.add_lines(lines + multistart_lines)
plot.set_labels(
x_label="No. of criterion evaluations",
y_label="Criterion value"
)
plot.set_legend_position(legend=CRITERION_PLOT_BACKEND_TO_LEGEND[backend])
return plot.backend_native_figureThere was a problem hiding this comment.
To update the legend we should probably add a
set_legend_positionmethod.
I agree, although I think set_legend_props might be a better fit as it can also be used for any additional properties specific to the legend.
Where do we need to set the template? Can we set the template in the init?
- plotly requires it to be after creating the figure and
- matplotlib requires it before creating the figure.
As we are creating the figure within the init, we can also set template in init.
Summary of Changes
backendparameter tocriterion_plot()which accepts "plotly" or "matplotlib"criterion_plot()now returns the figure object corresponding to the chosen backend.backends.pywhich serves as a registry for all backendsTo-Do
It is very much still a work in progress.
Plot